Skip to content

feat(client): [internal] container extensions and presence use #24399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

jason-ha
Copy link
Contributor

@jason-ha jason-ha commented Apr 18, 2025

ContainerRuntime path-based Signal addressing

For Signals, support using a / prefixed address in IEnvelope (at ContainerRuntime). No change to Ops.

  • Only extension Signals are sent with / prefix.
  • Receiving is supported for /ext/ (as well as /runtime/ and /channels/ for uniformity).

Refactor signal submission so that application of applyTrackingToBroadcastSignalEnvelope is centralized under sequenceAndSubmitSignal and use UnsequencedSignalEnvelope to make clientBroadcastSignalSequenceNumber stamping (or lack of) more obvious.

ContainerRuntime.submitSignalFn confirms envelopes coming through don't start with / that is reserved for path-based addressing.

Signal message types promoted to generics

ISignalEnvelope and I*SignalMessage* related types accept type and content pairs to form tight association between them.

[internal] ContainerExtension API

Refactor the prototype API from presence package to container-runtime-definitions.

Extensions are registered with ContainerRuntime (via support from FluidContainer in fluid-static in declarative API) and currently may only send and receive Signals. Encapsulated support will require further work to define appropriate cross-layer interfaces. fluid-static support will likely change to register presence directly but does not currently.

Build out extension message types to define outbound and inbound variants with inbound further divided into raw (unverified) and verified versions. Extensions may only submit messages conforming to the types they specify. The ExtensionHost (aka ContainerRuntime will deliver typed messages via processSignal but in unverified form.

core-interfaces additions

  • TypedMessage added for Signal message generics constraint type.
  • [internal] BrandedType added to support distinguishing raw from verified messages.
  • [internal] InternalUtilityTypes.FlattenIntersection exposed and fixed up for extension message types.

presence changes

  • Adapt internal IEphemeralRuntime to mostly match ExtensionHost.
  • Separate message types to protocol.ts and use more broadly.
    • Update test types (and some data) to conform to the stricter requirements
  • Improve incoming signal validation:
    • Check for exactly known type names
    • Allow for "optional" unrecognized signal; if a signal is not optional it must be recognized to avoid assert (throw)

Support getPresence(container: IFluidContainer)

  • Update Container and ContainerRuntime to support extensions.
  • Replace getPresenceViaDataObject use in examples and tests

@Copilot Copilot AI review requested due to automatic review settings April 18, 2025 00:58
@jason-ha jason-ha requested review from a team as code owners April 18, 2025 00:58
@jason-ha jason-ha requested review from anthony-murphy, markfields, tylerbutler, vladsud and WillieHabi and removed request for Copilot April 18, 2025 00:58
@github-actions github-actions bot added base: main PRs targeted against main branch area: examples Changes that focus on our examples area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: loader Loader related issues area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc area: website changeset-present public api change Changes to a public API labels Apr 18, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces support for path-based Signal addressing and centralizes signal submission logic through refactored APIs and generic message types. Key changes include:

  • Migrating signal submission and validation to use a centralized helper and updated types.
  • Removing deprecated container extension definitions and unused APIs.
  • Updating examples and documentation to align with the new Presence APIs.

Reviewed Changes

Copilot reviewed 39 out of 40 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/framework/presence/src/datastorePresenceManagerFactory.ts Introduces emitter-based event wiring and updates signal validation to use RawInboundExtensionMessage.
packages/framework/presence/src/container-definitions/*.ts Removes unused container extension interfaces and definitions.
packages/common/driver-definitions/src/protocol/protocol.ts Updates signal message types to use generics with TypedMessage.
packages/common/core-interfaces/* Adds and updates definitions for TypedMessage, ISignalEnvelope, and FlattenIntersection.
packages/common/container-definitions/* Exposes new extension runtime and container extension support.
examples/* Updates example usage from getPresenceViaDataObject to getPresence and refines related comments.
docs/docs/build/presence.md Revises onboarding instructions and fixes a typo in the documentation.
.changeset/wet-towns-ask.md Adds a changeset detailing the new path-based addressing scheme.
Files not reviewed (1)
  • packages/framework/presence/package.json: Language not supported
Comments suppressed due to low confidence (1)

packages/common/driver-definitions/src/protocol/protocol.ts:346

  • [nitpick] Ensure that the renaming and generic update to ISignalMessageBase and related types aligns with all downstream consumers to avoid type mismatches.
export interface ISignalMessageBase<TMessage extends TypedMessage = TypedMessage> {

@jason-ha jason-ha requested review from ChumpChief and alexvy86 April 18, 2025 00:59
jason-ha and others added 6 commits April 24, 2025 17:25
For Signals, support using a `/` separated address in `IEnvelope` (at ContainerRuntime). No change to Ops.
- Reading is always supported.
- Writing (sending) for all signals requires `IContainerRuntimeOptionsInternal.enablePathBasedAddressing` to be enabled and is not externally exposed nor internally enabled apart from testing.

Refactor signal submission so that application of `applyTrackingToBroadcastSignalEnvelope` is centralized and use `UnsequencedSignalEnvelope` to make `clientBroadcastSignalSequenceNumber` stamping (or lack of) more obvious.

Add two helpers:
- `submitWithPathBasedSignalAddress` will promote addressing to always use paths.
- `submitAssertingLegacySignalAddressing` confirms envelopes coming through don't start with `/` that is reserved for path-based addressing.

docs(client): changeset format update
and additional comment.

style(client): shorten option name

improvement(client-contain-runtime): remove pathBasedAddressing from DocSchema impact
and use to type ContainerRuntime signals received.
refactor `ContainerExtension` interface from `presence` with several updates to `ExtensionRuntime` and accommodating `presence` changes.
…n messages

across submitting and processing signals.

+ Improve incoming signal validation:
  - Check for exactly known type names
  - Allow for "optional" unrecognized signal; if a signal is not optional it must be recognized to avoid assert (throw)
+ Clean up test data types and content to conform
from [at least partially] validated/verified ones
and use in presence examples and tests

docs: fix presence.md typo

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@jason-ha jason-ha force-pushed the presence/infra/support-path-based-address-routing-for-extensions branch from cb8b9d3 to 112853b Compare April 25, 2025 00:39
Copy link
Contributor

@ChumpChief ChumpChief left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll pause on deeper review, I think the two big comments are core to the idea of this PR and so probably want to hash those out first.

// Note that this pattern allows for ! prefix in top-address but
// does not give it any more special treatment than without it.
const topAddressAndSubaddress = fullAddress.match(
/^\/(?<optional>[!?]?)(?<top>[^/]*)\/(?<subaddress>.*)$/,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've become more averse to path-based approaches recently, particularly as I've been working on handle-related stuff and hating their absolutePath. IMO most of our current path-like stuff is fallout from the request() pattern, which has generally just been a source of pain for us.

Is it necessary to collapse routing information into a string? Could we send it as a structured object instead, or wrap envelopes, etc.? I would love if when this function looks at the envelope, it simply has the information it wants already without needing to do any parsing/transformation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion to continue in internal Team chat

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've kept using the existing address string as the full routing information but removed all of the pattern complexity so that it is just split by /. There is no plan to extend this to ops currently where structuring might be more useful.
string is kept because we need (want) a string address anyway to play nice(r) with older client that happen to see these otherwise unexpected signals.

for explicit simplicity and correctness when no UseContext
@github-actions github-actions bot removed the dependencies Pull requests that update a dependency file label May 23, 2025
@jason-ha jason-ha force-pushed the presence/infra/support-path-based-address-routing-for-extensions branch from ee27d5d to 46fa5bf Compare May 23, 2025 09:42
@jason-ha jason-ha marked this pull request as ready for review May 23, 2025 09:43
@jason-ha jason-ha requested review from ChumpChief and markfields May 23, 2025 09:43
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label May 23, 2025
Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  209835 links
    1663 destination URLs
    1895 URLs ignored
       0 warnings
       0 errors


Copy link
Member

@tylerbutler tylerbutler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving for docs.

@jason-ha
Copy link
Contributor Author

Dear reviewers, I believe that most concerns have been addressed here, but as many are out I am completing the PR to unblock Designer. Will address all feedback given here or in either of the internal Teams chats. Support for encapsulated model to be designed and reviewed.

@jason-ha jason-ha requested a review from a team May 23, 2025 23:27
Copy link
Contributor

@Josmithr Josmithr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving API changes


import type { IRuntimeInternal } from "@fluidframework/presence/internal/container-definitions/internal";
/**
* @internal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can we add semantic docs since this is package-exported?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not package exported

import type { SystemWorkspaceDatastore } from "./systemWorkspace.js";

/**
* @internal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: docs for package-exported members

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not package exported. It is just inter package file "exported".

/**
* Brand for value that has not been verified.
*
* Usage:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit

Suggested change
* Usage:
* @remarks
* Usage:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix in post

@jason-ha jason-ha merged commit 5c6824a into microsoft:main May 24, 2025
63 checks passed
jason-ha added a commit to jason-ha/FluidFramework that referenced this pull request May 28, 2025
- removes @internal where API is not package exported and adding real notes where none were.
- adds documentation to exported APIs

Follow-up for microsoft#24399 and microsoft#24710
jason-ha added a commit that referenced this pull request May 30, 2025
- removes `@internal` where API is not package exported and adding real
notes where none were.
- adds documentation to exported APIs

Follow-up for #24399 and #24710
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: contributor experience area: examples Changes that focus on our examples area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: loader Loader related issues area: runtime Runtime related issues area: website base: main PRs targeted against main branch changeset-present dependencies Pull requests that update a dependency file public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants